Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

select a pre-release if it's specified in the Gemfile #6570

Merged
merged 1 commit into from
Jun 20, 2018

Conversation

akihiro17
Copy link
Contributor

@akihiro17 akihiro17 commented Jun 7, 2018

What was the end-user problem that led to this PR?

The problem was #6449

Closes #6449

What was your diagnosis of the problem?

My diagnosis was that Bundler didn't select a pre-release even if one of the dependencies of a gem was specified with a pre-release version in the Gemfile.

What is your fix for the problem, implemented in this PR?

My fix is to change Bundler::Resolver#requirement_satisfied_by? so that it does not reject a pre-release if at least one of the dependencies is pre-released.

Why did you choose this fix out of the possible options?

@ghost
Copy link

ghost commented Jun 7, 2018

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

@segiddins
Copy link
Member

Thanks!

@greysteil any objections to me merging this?

@greysteil
Copy link
Contributor

@akihiro17 - nice work on this! It's a thorny part of the resolution process to be digging into. I've confirmed locally that this fixes the issue and the test is 👌.

As I understand it, the logic here is "allow a pre-release version for a non-pre-release requirement if any of the sub-dependency requirements of that pre-releases version specify pre-releases".

I don't think that logic makes sense, and think this might instead be working because actually we can remove line 186-189 entirely. It was added before #6014 was, and I think #6014 should do everything required to ensure pre-releases aren't selected when another option is possible.

What do you reckon? Am I missing something in the logic here?

@akihiro17
Copy link
Contributor Author

Thank you for your review.

I think we can remove pre-release logic(line 186-19) from requirement_satisfied_by? because #6014 seems to avoid pre-release when a non-pre-release version is possible.

@greysteil
Copy link
Contributor

Awesome - do you want to update this PR with that change? I'm then 👍. @segiddins - sound good to you?

@akihiro17
Copy link
Contributor Author

akihiro17 commented Jun 18, 2018

do you want to update this PR with that change?

I'll update the PR later.

Thanks.

because rubygems#6014 avoids pre-releases when a non-pre-release version is possible
@segiddins
Copy link
Member

This seems fine? But with this sort of changes, I've learned it's hard to be 100% sure ahead of time

@bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit 01a2200 has been approved by segiddins

@bundlerbot
Copy link
Collaborator

⌛ Testing commit 01a2200 with merge a97917c...

bundlerbot added a commit that referenced this pull request Jun 20, 2018
select a pre-release if it's specified in the Gemfile

### What was the end-user problem that led to this PR?

The problem was #6449

Closes #6449

### What was your diagnosis of the problem?

My diagnosis was that Bundler didn't select a pre-release even if one of the dependencies of a gem was specified with a pre-release version in the Gemfile.

### What is your fix for the problem, implemented in this PR?

My fix is to change `Bundler::Resolver#requirement_satisfied_by?` so that it does not reject a pre-release if at least one of the dependencies is pre-released.

### Why did you choose this fix out of the possible options?
@bundlerbot
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: segiddins
Pushing a97917c to master...

@bundlerbot bundlerbot merged commit 01a2200 into rubygems:master Jun 20, 2018
@colby-swandale colby-swandale added this to the 1.16.4 milestone Jul 17, 2018
colby-swandale pushed a commit that referenced this pull request Aug 16, 2018
select a pre-release if it's specified in the Gemfile

### What was the end-user problem that led to this PR?

The problem was #6449

Closes #6449

### What was your diagnosis of the problem?

My diagnosis was that Bundler didn't select a pre-release even if one of the dependencies of a gem was specified with a pre-release version in the Gemfile.

### What is your fix for the problem, implemented in this PR?

My fix is to change `Bundler::Resolver#requirement_satisfied_by?` so that it does not reject a pre-release if at least one of the dependencies is pre-released.

### Why did you choose this fix out of the possible options?

(cherry picked from commit a97917c)
colby-swandale added a commit that referenced this pull request Aug 22, 2018
* 1-16-stable:
  Version 1.16.4 with changelog
  Auto merge of #6668 - eregon:fix-encoding-spec-from-6661, r=deivid-rodriguez
  Add encoding magic comment to gemfile spec
  Auto merge of #6662 - bundler:indirect/update-authors, r=colby-swandale
  Auto merge of #6650 - greysteil:dont-mutate-original-trees, r=segiddins
  Auto merge of #6661 - eregon:consistent-encoding-for-reading-files, r=deivid-rodriguez
  Auto merge of #6652 - bundler:seg-molinillo-0.6.6, r=segiddins
  Auto merge of #6645 - bundler:colby/require-etc, r=colby-swandale
  Auto merge of #6636 - ojab:1-16-stable, r=indirect
  Auto merge of #6624 - bundler:no-document, r=colby-swandale
  Auto merge of #6621 - ralphbolo:patch-1, r=segiddins
  Auto merge of #6613 - kemitchell:mention-show-sorts-in-doc, r=colby-swandale
  Auto merge of #6570 - akihiro17:prerelease-dependency, r=segiddins
  Auto merge of #6568 - greysteil:conservative-groups, r=segiddins
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when resolving dependencies
5 participants